[TSPS-512] Implement actions column w/ downloads outputs and view error modals #5339
Conversation
| <div style={{ fontWeight: 'bold', marginTop: '2rem' }}>Our current offerings:</div> | ||
| <div> | ||
| <span style={{ fontStyle: 'italic' }}>All of Us</span> + AnVIL Imputation Service | ||
| <a |
There was a problem hiding this comment.
This is an unrelated drive-by change to link to the marketing page from the landing page
There was a problem hiding this comment.
Pull Request Overview
This PR adds modal components to display pipeline outputs and errors, updates the job history table to include a deletion date column, and adjusts styles and API models to support these features. Key changes include:
- Introducing ViewOutputsModal and ViewErrorModal components for displaying pipeline outputs and error details.
- Updating JobHistory to include a new column for data deletion date and new action buttons that trigger the appropriate modals.
- Adding new API interfaces and a method in Teaspoons for retrieving pipeline run results.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/scientificServices/pipelines/views/modals/ViewOutputsModal.tsx | New modal for showing pipeline output files with download support. |
| src/pages/scientificServices/pipelines/views/modals/ViewErrorModal.tsx | New modal for displaying detailed pipeline error messages and causes. |
| src/pages/scientificServices/pipelines/views/JobHistory.tsx | Updated job history table with a deletion date column and action buttons triggering modals. |
| src/pages/scientificServices/landingPage/ScientificServicesDescription.tsx | Modified link element for external service with proper accessibility attributes. |
| src/libs/ajax/teaspoons/teaspoons-models.ts | Added new interfaces to support detailed pipeline run reporting. |
| src/libs/ajax/teaspoons/Teaspoons.ts | Introduced getPipelineRunResult API method for fetching pipeline run results. |
| const outputsModal = useModalHandler(() => { | ||
| return <ViewOutputsModal jobId={pipelineRun.jobId} onDismiss={outputsModal.close} />; | ||
| }); | ||
|
|
||
| const errorModal = useModalHandler(() => { | ||
| return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />; |
There was a problem hiding this comment.
[nitpick] Consider defining the onDismiss callback outside of the modal handler to avoid a self-referential closure, which can improve code clarity.
| const outputsModal = useModalHandler(() => { | |
| return <ViewOutputsModal jobId={pipelineRun.jobId} onDismiss={outputsModal.close} />; | |
| }); | |
| const errorModal = useModalHandler(() => { | |
| return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />; | |
| const onOutputsModalDismiss = () => outputsModal.close(); | |
| const outputsModal = useModalHandler(() => { | |
| return <ViewOutputsModal jobId={pipelineRun.jobId} onDismiss={onOutputsModalDismiss} />; | |
| }); | |
| const onErrorModalDismiss = () => errorModal.close(); | |
| const errorModal = useModalHandler(() => { | |
| return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={onErrorModalDismiss} />; |
| const errorModal = useModalHandler(() => { | ||
| return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />; |
There was a problem hiding this comment.
[nitpick] Consider refactoring the onDismiss callback so that it does not reference errorModal directly within its own initialization, enhancing readability.
| const errorModal = useModalHandler(() => { | |
| return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={errorModal.close} />; | |
| const handleErrorModalDismiss = () => { | |
| errorModal.close(); | |
| }; | |
| const errorModal = useModalHandler(() => { | |
| return <ViewErrorModal jobId={pipelineRun.jobId} onDismiss={handleErrorModalDismiss} />; |
jsotobroad
left a comment
jsotobroad
left a comment
There was a problem hiding this comment.
i really like the download outputs modal
| <h3>Job History</h3> | ||
| <div>All files associated with jobs will be automatically deleted after 2 weeks from completed.</div> | ||
| <div style={{ marginBottom: '0.25rem' }}> | ||
| All files associated with jobs will be automatically deleted after 2 weeks from completed. |
There was a problem hiding this comment.
after 2 weeks from completed -> 2 weeks after completion ?
|
|
||
| const completionDate = new Date(pipelineRun?.timeCompleted); | ||
| const deletionDate = new Date(completionDate); | ||
| deletionDate.setDate(deletionDate.getDate() + 14); |
There was a problem hiding this comment.
this is something we already return as part of the PipelineRunReport which you would get from a call to /result on a successful pipeline run. I guess we should decide if we want to only show them this date after they click on the download button in the overlay or do this math here and the response's time in the overlay or show them the response's time as part of the list pipeline runs call
There was a problem hiding this comment.
oh yeah, i went back and forth on this. I think since it's related to permanently deleting data that they paid to generate, it's good UX to have it here and in the download outputs modal. I also think we should provide an additional visual cue in the cell if the data is facing imminent deletion (perhaps <72 hours away from deletion).
I didn't want to scope-explode so I just added the hardcoded +14 days here, but i guess we could take one of these approaches in a follow-up ticket:
- add the deletion date to the listPipelineRuns call, like you suggested (I'm leaning towards this)
- return the TTL config value somewhere so we can do the math here based on that, and we could also programmatically generate the "All files associated with jobs will be automatically deleted after X days from completion" text based on that value
I guess it also depends on if we ever think we'd change the global TTL, or perhaps have different TTLs per pipeline
There was a problem hiding this comment.
yea i woudl say lets make a followup ticket to discuss what UX we want around this and the rest of this looks good to me
| </ButtonPrimary> | ||
| </div> | ||
| ))} | ||
| {result.pipelineRunReport.outputExpirationDate && ( |
There was a problem hiding this comment.
note to self: it would probably be good to add some logic here to check if the expiration date has passed, and disable the download buttons + show different text
There was a problem hiding this comment.
(will also cover this in a followup UX ticket, since it requires further discussion)
MatthewBemis
force-pushed
the
mb-tsps-512
branch
from
a1a8560 to
2f65207
Compare
|
MatthewBemis
merged commit f30aab7
into
tsps_imputation_ui_feature_branch
Jira Ticket: https://broadworkbench.atlassian.net/browse/TSPS-512 and https://broadworkbench.atlassian.net/browse/TSPS-514
Summary of changes:
What
Why
Testing strategy